Retrying is pretty specific to the particular KVStore implementation. I don't think we should expose retrying params on top of what we already do for implementations internally.

Sure thing. This has been dropped.
I considered it because users can choose their own KVStore implementation without considering retrying. I thought to add some level of control but the added complexity and size of the changeset might not be worth it.

nit: Might also be local.

Updated and addressed here db1fe83

Can we now also expose Builder::build_with_store?

Yes we can.
This is exposed here 451a392

If we rather make this an impl From<Arc<dyn LdkSyncAndAsyncKVStore + Send + Sync>> for Arc .., we can drop the wrap_storemacro and just use.into()` instead.

I think we need to use build_with_store_internal here to make sure we're using the same Runtime etc.

I'm confused, why are we replicating all the write-ordering logic here? Given the implementations are required to fullfill LDK's requirements already, can't we just call the inner write and be done with it?

Again, the reasoning here is that users might not implement the KVStore trait for their own stores as required and thus, the onus of correctness will fall to us. If we can't get it from their inner stores, the wrapper TierStore should provide some guarantees. I do agree that if the inner store already satisfies LDK's requirements, the wrapper shouldn't need to duplicate that logic. The write-ordering has been removed.

I think here (and in remove) we need to add the write-ordering logic to ensure we follow LDK's KVStore::write requirements.

This has now been added 1ba4831

This is odd. Why do we need this?

I had initially used the same name and the feature gating became necessary to differentiate the types at call sites when uniffi was enabled.

Not quite sure what coverage we gain with DelayedStore? IMO, we might be better off dropping all this boilerplate.

This has been removed here cb66b59.
I couldn't think of any way better to test backup queue overflow not impacting primary writes. I agree that the boilerplate seems too much for just that single case.

Hmm, any reason this needs to be separate from DynStore? Couldn't we merge them?

At the time I believe I introduced the extra wrapper due to the limitation of foreign trait implementation on foreign types, i.e. I couldn't implement KVStore for Arc<dyn LdkSyncAndAsyncKVStore> but with the new changes, this is no longer relevant and has been removed.

Probably not worth taking a dependency just to desugar impl Future<Output = Result<(), IOError>> + Send + Sync + 'a.

Probably not worth taking a dependency just to desugar impl Future<Output = Result<(), IOError>> + Send + Sync + 'a.

Well, it's 'the official'/supported way to do async traits with Uniffi: https://mozilla.github.io/uniffi-rs/latest/futures.html#exporting-async-trait-methods

Does uniffi require that in some way? Its just a comically-overkill way to sugar impl Future which is kinda nuts...

Oh, worse, it looks like async_trait desugars to the Pin<Box<dyn ...>> version...which is dumb but at least for uniffi it shouldn't matter cause we'd have to do that anyway.

Let's not introduce this in the first place then. Warnings are fine to have for a commit.

KVStore, not KVStoreSync, right?

Hmm, is this really what we want? If the backup store is remote and we fail to persist, should we really abort? Doesn't make this our operations less reliable in practice instead of more, if we always fail on the weakest link?

Also, if we simply abort if one of the writes fails, how could we recover without inconsistencies? We don't have a good way to roll-back the write that went through, right? So we'd still have inconsistencies, so wouldn't it be preferable to not fail the write in the first place?

Hmm, is this really what we want? If the backup store is remote and we fail to persist, should we really abort? Doesn't make this our operations less reliable in practice instead of more, if we always fail on the weakest link?

The current implementation follows Matt's recommendation, where we assume a simplified configuration with primary-remote and backup-local. In this case, we should have high confidence in local backup rarely failing. However, if it does, I do agree that we don't want to propagate its error, especially if primary-remote succeeds. I could modify it so that we log backup-local failures and only ever fail if primary fails.

Also, if we simply abort if one of the writes fails, how could we recover without inconsistencies? We don't have a good way to roll-back the write that went through, right? So we'd still have inconsistencies, so wouldn't it be preferable to not fail the write in the first place?

I have thought about this and wonder if we should explore syncing by extending DynStoreTrait with list_all_keys and use LDK's existing migrate_kv_store_data to reconcile backup from primary at startup. This would require that primary and backup both impl MigratableKVStore. I'd like to know what you think about this approach.

Regarding primary-local, backup-remote, Matt suggested that I shelve the consideration for a later, possible follow up to this, if we can come up with a more robust solution than I initially implemented. Any pointers here will be deeply appreciated.

The current implementation follows Matt's recommendation, where we assume a simplified configuration with primary-remote and backup-local. In this case, we should have high confidence in local backup rarely failing. However, if it does, I do agree that we don't want to propagate its error, especially if primary-remote succeeds. I could modify it so that we log backup-local failures and only ever fail if primary fails.

Hum, maybe it might be best to discuss all options we have offline @TheBlueMatt ? Especially if we also want to support remote backup (which is reasonable/important, IMO), failing if either operation fails seems not great? However, if we only want to support a local mirror for now, maybe we should just simplify everything, and make enabling the backup store a binary option that enables an SQLite-based backup store? As a side effect, the user could easily copy the database to recover?

I have thought about this and wonder if we should explore syncing by extending DynStoreTrait with list_all_keys and use LDK's existing migrate_kv_store_data to reconcile backup from primary at startup. This would require that primary and backup both impl MigratableKVStore. I'd like to know what you think about this approach.

Yeah, likely using MigratableKVStore is the way to go, but maybe we should just leave this as a manual step rather than doing auto-recovery? We do already expose all necessary utilities for it, no?

Regarding primary-local, backup-remote, Matt suggested that I shelve the consideration for a later, possible follow up to this, if we can come up with a more robust solution than I initially implemented. Any pointers here will be deeply appreciated.

Yeah, okay, if we really don't want to do remote backups for now, we should just keep a mirrored SQLite store at a configurable location, IMO, and not allow the user to configure anything else. I do however want to note that people might still try to misuse this, e.g., by pointing the storage location of the mirror to a remote file system (ask me how I know: I did the same thing for a while to keep my CLN node at the time backed up).

IMO if we want to move away from failing if either backend fails we need to have proper recovery logic in place. If we have a remote backup and writing to it fails, it otherwise wouldn't be obvious that restoring from the "live backup" is suddenly unsafe. Even making the write complete early suddenly makes using the "live backup" unsafe after a shutdown if we were mid-write.

It's great in theory to provide an optimistic live backup, but absent a way to actually use it safely in really not convinced it's a great idea to ship it.

Right, so IMO the short term solution is to hardcode backups to local SQLite, and treat write failures the same as persistence failures to the primary store.

Ofc, that won't safe us entirely from the two stores getting out of sync as we'd still won't have rollbacks in place in case one write succeeds and the other doesn't. Question is if that means that we can at least parallelize instead of sequencing the writes.

That should be fine, though, as long as async writes aren't completed until both complete. IOW, it should always be the case that we can restart in either state when there's a pending uncompleted write when we stop. I don't see any reason to not parallelize the writes.

I guess we didn't strictly have any kind of guarantees around it being allowed for write A to be incomplete, then on restart missing, then later completed (in case we switched the "primary store" after a later restart). At least in the lightning crate I think that's fine - for monitors it'd only matter in cases where another update happened, but that would imply overwriting the previous dangling write to A. Similar is obviously true for nearly everything else, where we're constantly overwriting the latest state.

I guess we should document this on KVStore so that its a formal part of the API.

Okay, as mention above, I noted that: but what can I do now if a write failed? How can the user actually take action to avoid this or recover from it?

With the proposed change (only fail if primary fails), the user never needs to act on backup failures, we can just log them and reconcile at startup. Primary failure handling remains as is.

I have spent some time rethinking this and considered the option of allowing users to configure how they'd like to backup data, i.e. either best effort or semi-synchronously. In the latter case, in addition to the synchronous backup store, I have added an asynchronous retry queue backed by a local FilesystemStoreV2 where we try to durably persist our backup intent.

We offer guarantees in semi-sync mode based on the durable persistence of primary write and backup intent, surfacing a write failure to the caller only if the backup intent cannot be persisted to the local retry queue, even though the primary write may have already succeeded. I believe this is better than the alternative where we silently lose backup coverage without the caller knowing.

This does create a scenario where the backup store can drift behind the primary: if the node crashes after the primary write succeeds but before backup intent to retry queue is persisted, the backup will be missing those operations. The backup will be internally consistent but stale, and drift between the stores will form as the retry queue will be unaware of backup intent(s) that didn't persist. I'd like to understand whether this guarantee is sufficient for the restore use case, or whether we'd also need active reconciliation between the stores.

This approach was inspired by how DDIA describes semi-synchronous replication in single-leader replication systems. I also think it addresses some of the concerns @TheBlueMatt raised about primary-local and backup-remote configurations.

Would be really grateful to get both your thoughts on the approach taken.

Thanks for thinking through this, however, it would be great if we could move any more advanced scheme like a backup queue to a follow-up PR as that might need some more review time and we should aim to land this PR (and the bindings follow-up) soon. So, as mentioned above, how about we just make the backup store an SQLite store with configurable location, requiring both writes to continue. Then, in a follow up we can explore the queuing concept.

For one that gives us more flexibility with regards to when this follow-up lands, but it would also allow us to further think through, e.g., how to deal with the overlap with a next-gen VssStore, which should also start using a local read cache, and how all of this would interact with the generally keeping only a LRU page cache of the (payment) stores in memory and reading the entries from the stores on demand (which we also want to finally do soon).

Why is this allowing dead_code?

I added this to silence warnings as I didn't think warnings were permissible in intermediate commits. Since you've clarified that they are, I can remove them from intermediate commits. However, the annotations on the final commit are still needed (the code isn't used except in #871), so CI will fail without them.

So here we set the backup store, but how do we envision the restore to work? Should that be part of the recovery_mode?

There are two approaches we can take here. For the first, have both primary and backup concrete stores implement MigratableKVStore and have the user call migrate_kv_store_data before building. It's simple but not ideal as it's not part of any existing node or builder APIs, and would require explicit documentation.

Alternatively, we can add a restore_from_backup(backup) method on NodeBuilder and have the migration/restoration of data from backup (source) to primary (target) happen inside build. This requires adding list_all_keys to DynStoreTrait, and MigratableKVStore for both stores so the migration can work through the type-erased Arc<DynStore> layer.

For the second approach, we could also refactor recovery_mode from a bool into a struct:

pub struct RecoveryMode {
    pub wallet: bool, // resync on-chain wallet from genesis
    pub backup: bool, // restore persisted data from backup store
}

This keeps both recovery concerns under one concept while remaining independent. A user restoring from backup may not need a full wallet resync, and vice versa.

On the restore side, I went with the second approach from my earlier comment restore_from_backup() on the builder, with restoration happening inside build before normal startup reads. I refactored recovery_mode from a bool into a RecoveryConfig struct that keeps wallet recovery and backup restore independent. The restore itself enumerates the backup store via list_all_keys, filters to a known durable key inventory, checks that the primary store is empty, and copies only matching entries. This keeps restore policy explicit rather than blindly migrating everything from the backup store.